Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Support storage size unit and show error message correctly #1714

Merged
merged 1 commit into from
Sep 13, 2023
Merged

Support storage size unit and show error message correctly #1714

merged 1 commit into from
Sep 13, 2023

Conversation

DaoDaoNoCode
Copy link
Member

@DaoDaoNoCode DaoDaoNoCode commented Aug 29, 2023 •
edited
Loading

Closes #1480 in commit 1
Closes #1565 in commit 2

Description

  1. Refactoring the storage section in the storage modal and the workbench spawner page (they reuse the same component), adding unit support (Gi and Mi) for the storage size, and only allowing the users to input integers.
  2. Add a dry run for the storage modal network calls, it can now throw errors as expected (Address the comment in Show error message correctly in manage storage modal #1679)

On the spawner page:
Screenshot 2023-08-29 at 9 39 19 AM

In storage modal:
Screenshot 2023-08-29 at 9 40 52 AM

How Has This Been Tested?

  1. Go to the data science project and create a workbench with the storage
  2. Try to edit the storage, give it a value smaller than the current size
  3. You should be able to see the error alert in the modal without closing the modal
  4. Try to change the size with different units, for example, if the original size is 1 Gi, try to change it to 2000 Mi
  5. You should be able to make the change, and when you try to edit the storage, the size should be set correctly

Test Impact

N/A, mostly network call stuff and reused components

Request review criteria:

Self checklist (all need to be checked):

  • The developer has manually tested the changes and verified that the changes work
  • Commits have been squashed into descriptive, self-contained units of work (e.g. 'WIP' and 'Implements feedback' style messages have been removed)
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has added tests or explained why testing cannot be added (unit tests & storybook for related changes)

If you have UI changes:

  • Included any necessary screenshots or gifs if it was a UI change.
  • Included tags to the UX team if it was a UI/UX change.

After the PR is posted & before it merges:

  • The developer has tested their solution on a cluster by using the image produced by the PR to main

@DaoDaoNoCode
Copy link
Member Author

@xianli123 Could you check the UI changes?

@DaoDaoNoCode DaoDaoNoCode mentioned this pull request Aug 29, 2023
Closed
7 tasks
@xianli123
Copy link

@DaoDaoNoCode Thanks for doing that. It looks good to me.

@openshift-ci openshift-ci bot added the lgtm label Aug 30, 2023
@openshift-ci openshift-ci bot removed the lgtm label Sep 1, 2023
@DaoDaoNoCode
Copy link
Member Author

This PR should go to f/ds-projects branch first, redirecting...

@DaoDaoNoCode DaoDaoNoCode changed the base branch from main to f/ds-projects September 5, 2023 13:24
@DaoDaoNoCode DaoDaoNoCode mentioned this pull request Sep 5, 2023
Merged
7 tasks
@andrewballantyne andrewballantyne added the pr/no-tests-allowed Added by an official approver - this PR is allowed no tests. Omitted, a test must accompany the PR label Sep 7, 2023
christianvogt
christianvogt reviewed Sep 8, 2023
View reviewed changes
frontend/src/pages/projects/screens/detail/storage/ManageStorageModal.tsx Outdated
updatePvcDescription(pvcName, namespace, createData.nameDesc.description, { dryRun }),
);
}
if (getPvcTotalSize(existingData) !== createData.size) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If a previous request was made but the status hasn't yet updated, we are still making a patch request for the exact same request.

frontend/src/pages/projects/screens/detail/storage/ManageStorageModal.tsx
import ExistingConnectedNotebooks from './ExistingConnectedNotebooks';

import './ManageStorageModal.scss';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's odd to me that there is a generic style in ManageStorageModal.scss and this file isn't even used by ManageStorageModal.tsx. It would make more sense to move those styles to a more generic location.

Including these styles in App.scss or a new overrides.scss file makes more sense. cc @andrewballantyne

frontend/src/pages/projects/screens/detail/storage/ManageStorageModal.tsx Outdated
Comment on lines 85 to 74
const pvcName = existingData.metadata.name;
if (getPvcDisplayName(existingData) !== createData.nameDesc.name) {
pvcPromises.push(
updatePvcDisplayName(pvcName, namespace, createData.nameDesc.name, { dryRun }),
);
}
if (getPvcDescription(existingData) !== createData.nameDesc.description) {
pvcPromises.push(
updatePvcDescription(pvcName, namespace, createData.nameDesc.description, { dryRun }),
);
}
if (getPvcTotalSize(existingData) !== createData.size) {
pvcPromises.push(updatePvcSize(pvcName, namespace, createData.size, { dryRun }));
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A change to name, description and size results in 3 separate patch request and dry runs for a total of 6 network calls. There should only be a single dry run followed by a single patch.

@DaoDaoNoCode
Copy link
Member Author

@christianvogt Updated the code based on your comments, can you re-review it when you get a chance?

@christianvogt
Copy link
Contributor

Re-tested. Seems to be working as expected.
Please squash the commits.

@DaoDaoNoCode
support unit for notebook storage
6dc30ef 
dry run and show error message on storage modal
@DaoDaoNoCode
Copy link
Member Author

@christianvogt Squash, this PR could be merged anytime because it's not going to the main branch.

@christianvogt
Copy link
Contributor

/lgtm
/approve

@openshift-ci openshift-ci bot added the lgtm label Sep 13, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 13, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: christianvogt, xianli123

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot merged commit 4b05222 into opendatahub-io:f/ds-projects Sep 13, 2023
@DaoDaoNoCode DaoDaoNoCode mentioned this pull request Sep 28, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@christianvogt christianvogt christianvogt left review comments

@xianli123 xianli123 xianli123 approved these changes

@alexcreasy alexcreasy Awaiting requested review from alexcreasy

@Gkrumbach07 Gkrumbach07 Awaiting requested review from Gkrumbach07

Assignees

@christianvogt christianvogt

@xianli123 xianli123

Labels
approved lgtm pr/no-tests-allowed Added by an official approver - this PR is allowed no tests. Omitted, a test must accompany the PR
Projects
None yet
Milestone
No milestone
5 participants
@DaoDaoNoCode @xianli123 @christianvogt @andrewballantyne @openshift-merge-robot